-
-
Notifications
You must be signed in to change notification settings - Fork 612
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix orderModelv2 for nullable profile column #7907
base: main
Are you sure you want to change the base?
Conversation
@@ -464,7 +464,7 @@ func orderToModelv2(order *corepb.Order) (*orderModelv2, error) { | |||
Created: order.Created.AsTime(), | |||
BeganProcessing: order.BeganProcessing, | |||
CertificateSerial: order.CertificateSerial, | |||
CertificateProfileName: order.CertificateProfileName, | |||
CertificateProfileName: &order.CertificateProfileName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's surprising that the resulting orderModelv2
contains a pointer to the original corepb.Order
's CertificateProfileName
, so changing one will change the other.
It would be better to create a new string with a copy of the input, for avoidance of surprising behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jsha That would look like this?
func orderToModelv2(order *corepb.Order) (*orderModelv2, error) {
profile := &order.CertificateProfileName
om := &orderModelv2{
...
CertificateProfileName: profile,
}
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, that still results in a pointer that points to the original code. This is what I'm looking for:
profile := *order.CertificateProfileName
...
CertificateProfileName: &profile
@@ -400,7 +400,7 @@ type orderModelv2 struct { | |||
Error []byte | |||
CertificateSerial string | |||
BeganProcessing bool | |||
CertificateProfileName string | |||
CertificateProfileName *string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be an orderModelv2
struct comment to match similar wording on the crlShardModel
struct.
// CertificateProfileName is a pointer because it is a NULL-able column.
Change the type of the orderModelv2 CertificateProfileName field to be a pointer to a string, reflecting the fact that the underlying database column is nullable. Add tests to ensure that order rows inserted with either order model can be read using the other model.
Fixes #7906